Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview of scalar as a simcomp #1129

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Conversation

timofeymukha
Copy link
Collaborator

@timofeymukha timofeymukha commented Feb 7, 2024

This PR currently has a working preview of a scalar simulation component. Feel free to play around :-).

Some points

  • The case now doesn't own the scalar, and it is just a regular simcomp.
  • There is some awkwardness in the simcomp constructor, but this can be addressed.
  • Output can be added to fluid's, or redirected to a different file.
  • Slight update of scalar_scheme's deferred init, so that scalar_pnpn does not parse json.
  • The json passed to the scalar is only the simcomps json, not the whole parameter file.
  • Material properties for the scalar are injected into case.material_properties. This is not very pretty right now, as wrong output is given to the log in the beginning. Will be fixed.
  • The scalar uses its own solver configuration, separate from fluid. The idea is eventually to allow having the scalar component with the fluid at all (at the point where fluid is also a simcomp).
  • Fixes simcomp init order in the simcomp_executor and adds a json util for extracting array items (both will be cherrypicked to separate PRs)
  • the scalar_mms example is updated to the new settings.

I guess it is still somewhat an open question if we want to go this way. I like it, but one does need to realize that the burden of getting the order of simcomps is on the user here. For example, the probes simcomp crashed for me because it tried to find the "s" field during init, and it was init before the scalar :-). We have an order parameter, to deal with this, but the crashes one gets are difficult to understand as a user. Perhaps, there can still be some guards implemented, but I don't think we can get all possible combos.

Another question I have is regarding gs and coef. Scalar takes the from fluid, but I would like to separate the two. I guess creating and extra gs and coef is not a very good option? Should these be moved up to case, perhaps? Almost everything relies on coef in general, so if fluid will be non-obligatory, I feel like we need to have coef elsewhere.

Lastly, I am pondering on whether scalar_scheme should deal with json parsing or that should all be done in the simcomp. If so,
we will need to pass more stuff to the constructor. The main barrier is that some stuff has to be passed as json anyway, particularly, the source_terms object, which is then used to init the scalar_source_term component of the scheme. So maybe one just pass the whole json anyway...

@timofeymukha timofeymukha added enhancement New feature or request don't merge Don't merge yet! labels Feb 7, 2024
@timofeymukha
Copy link
Collaborator Author

Treat the review request mostly as request for attention :-).

@timofeymukha
Copy link
Collaborator Author

By the way, one consequence of this is that one can of course have several simcomps of the "scalar" type, so it in its own way provides multiple scalar capability. Perhaps not computationally ideal, but for 2-3 scalars probably fine. Need a bit more flexibility for it to work though: the field name should be selectable and there needs to be better handling of checkpointing I suppose (not attached to fluid)

@MartinKarp
Copy link
Collaborator

I like this PR, especially since it in principle adds support for several scalars (while not computationally perfect). The same would be true if we extend to Fluid, e.g. neknek would become more or less trivial.

Dow we all agree that "everything as simcomp" is a good direction to go? If so I think we should perhaps just go for it, starting with the scalar.

@njansson
Copy link
Collaborator

@timofeymukha Should we keep this for the 0.8 release?

@timofeymukha
Copy link
Collaborator Author

@timofeymukha Should we keep this for the 0.8 release?

Could be a bit ambitions for May, not sure. This is already in pretty deep merge hell :-).

@njansson
Copy link
Collaborator

@timofeymukha Should we keep this for the 0.8 release?

Could be a bit ambitions for May, not sure. This is already in pretty deep merge hell :-).

Fair enough, then we push it to 0.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't merge Don't merge yet! enhancement New feature or request
Projects
No open projects
Status: 🙅Skipped
Development

Successfully merging this pull request may close these issues.

3 participants